-
Notifications
You must be signed in to change notification settings - Fork 2
[Step1]리뷰 요청 드립니다! #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: kimjinwook
Are you sure you want to change the base?
Conversation
|
|
||
| import java.util.List; | ||
|
|
||
| public class GameUtil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Util 이라는 이름의 클래스는 적합하지 않은 것 같아요 :)
유틸 클래스는 보통 실용성이나 유용성에 중점을 둔 정적 메서드들을 모아두고 여러 곳에서 호출하는 형식으로 사용하며 여러 곳에서 호출하기 때문에 유틸 클래스는 공통적인 로직을 다루도록 만들어야하며 합니다.
개인적으로는 실제로는 이러한 유틸 클래스의 메서드를 변경했을 때 영향이 너무 크기 때문에 최대한 사용을 지양하는 것을 선호합니다 :)
| } | ||
| System.out.println("수고하셨습니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
파일 마지막에 엔터(개행문자)를 넣어주세요 :)
이유는 리뷰를 진행할 때 깃허브에서 경고메시지를 지우고 혹시 모르는 파일 읽기 오류에 대비하기 위함입니다.
좀 더 알고싶으시면 참고 링크를 보셔도 재밌을 것 같네요 :)
Intellij 를 사용하실 경우엔Preferences -> Editor -> General -> Ensure line feed at file end on save 를 체크해주시면파일 저장 시 마지막에 개행문자를 자동으로 넣어줍니다!
|
|
||
| private void showMenu() { | ||
| ScannerUtil scannerUtil = new ScannerUtil(); | ||
| System.out.println("게임을 새로 시작하려면 1, 종료하려면 2를 입력하세요."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 핵심 로직을 구현하는 코드와 UI를 담당하는 로직을 구분한다.
- UI 로직을 InputView, ResultView와 같은 클래스를 추가해 분리한다.
프로그래밍 요구사항 중에 위와 같은 요구사항이 있었는데요. 이 부분을 어떻게 구현할지 생각해보시면 좋을 것 같습니다.
|
|
||
| import java.util.List; | ||
|
|
||
| public class Referee { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Referee 클래스의 인스턴스는 어떤 상태(내부 필드)도 가지지 않기 때문에 이 클래스의 모든 객체들은 동일한 객체이겠네요 !
너무 많은 상태를 가지는 것은 좋은 설계가 아니지만, 아무것도 가지지 않는 방식 또한 바람직한 설계는 아닙니다.
적절한 상태를 가진 객체를 설계해보면 좋을 것 같아요.
다른 클래스들에도 함께 적용되는 피드백일 것 같습니다 :)
| public int countStrike(List<Integer> list1, List<Integer> list2) { | ||
|
|
||
| int count = 0; | ||
| for (int index = 0; index < 3; index++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
임의의 숫자를 사용하는 매직넘버는소스 코드를 읽기 어렵게 만드는데요 !
이 부분을 어떻게 개선할 수 있을까요? 관련 링크 남겨놓겠습니다 :)
|
|
||
| @Test | ||
| @DisplayName("두 개의 리스트에서 몇 개의 숫자가 같은지 확인") | ||
| void countBall() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 테스트 메서드에서 예외를 선언을 하는 특별한 이유가 없다면 지워주세요 :)
테스트 메서드에 예외 선언을 함으로 써해당 테스트가 실패할 수도 있다라는 느낌을 주는 것 같습니다.
| if (strike == 3) { | ||
| return strike + "스트라이크"; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
핵심 로직에 화면(콘솔 출력)에 대한 요구사항이 섞여있네요!
만약 화면 요구사항이 변하게 된다면 어떻게 될까요?
화면의 요구사항이 바뀌었지만 개발자가 수정해야하는 부분은 도메인의 핵심 로직입니다.
아래 요구사항을 반영해야하는 이유중 하나가 될 수 있겠네요 :)
핵심 로직을 구현하는 코드와 UI를 담당하는 로직을 구분한다.
|
|
||
| public String insertString() { | ||
|
|
||
| Scanner scanner = new Scanner(System.in); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scanner는 매우 비싼 자원입니다. 미리 생성해두고 재활용하도록 해보면 어떨까요 ?
static 메서드를 활용해보면 좋을 것 같아요.
No description provided.